Check MACH_PORT_VALID on return from pthread_mach_thread_np #3520

indragiek · 2023-12-27T20:52:00Z

📜 Description

pthread_mach_thread_np can return invalid values so check for those and return nullptr from ThreadHandle. That means changing the call sites for ThreadHandle::current() to check for a null pointer value.

💡 Motivation and Context

We don't have a solid repro for #3354 but this is one of the potential causes. In any case it doesn't hurt to add the check.

💚 How did you test it?

Run tests

📝 Checklist

You have to check all boxes before merging:

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

pthread_mach_thread_np can return invalid values so check for those and return nullptr from ThreadHandle

philipphofmann

@indragiek, good catch 👍 . CI is broken, and it would be great to add or change some tests for these changes if possible.

Sources/Sentry/SentryThreadHandle.cpp

Sources/Sentry/include/SentryThreadHandle.hpp

armcknight

Reading the logic again in SentryBacktrace.cpp which contains the callsite for the function we've modified is a bit unclear to me, given the name allExcludingCurrent. Reading it starting at the loop, it looks like we're going to do work on all the other threads except the current one, which is true, but we do still have to retain the current thread in order to know what thread we're running on, and the callsite tells me we threw out that info when we actually didn't.

TL;DR I would propose we rename allExcludingCurrent to currentAndOtherThreads.

Sources/Sentry/SentryThreadHandle.cpp

Sources/Sentry/include/SentryThreadHandle.hpp

philipphofmann

Once we have some tests, I will approve this PR. The rest LGTM.

…4076)

codecov · 2024-06-20T19:56:45Z

Codecov Report

Attention: Patch coverage is 81.13208% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.165%. Comparing base (1734d1b) to head (6fac4d6).
Report is 343 commits behind head on main.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #3520       +/-   ##
=============================================
+ Coverage   89.241%   91.165%   +1.923%     
=============================================
  Files          529       609       +80     
  Lines        57780     47846     -9934     
  Branches     20687         0    -20687     
=============================================
- Hits         51564     43619     -7945     
+ Misses        5300      4227     -1073     
+ Partials       916         0      -916

Files	Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm	`90.714% <100.000%> (+3.558%)`	⬆️
Sources/Sentry/SentryThreadHandle.cpp	`62.727% <100.000%> (-6.504%)`	⬇️
Sources/Sentry/SentryTransactionContext.mm	`100.000% <100.000%> (ø)`
...yCrash/Recording/Monitors/SentryCrashMonitorType.c	`0.000% <ø> (ø)`
...entryCrash/Recording/Tools/SentryCrashSignalInfo.c	`100.000% <ø> (ø)`
...sts/SentryProfilerTests/SentryThreadHandleTests.mm	`95.789% <100.000%> (+2.932%)`	⬆️
Tests/SentryTests/SentryCrash/CrashReport.swift	`100.000% <100.000%> (+6.666%)`	⬆️
Tests/SentryTests/SentryInterfacesTests.m	`100.000% <100.000%> (ø)`
...rding/Monitors/SentryCrashMonitor_CPPException.cpp	`20.408% <0.000%> (-10.223%)`	⬇️
Tests/SentryProfilerTests/SentryBacktraceTests.mm	`95.744% <88.888%> (+0.939%)`	⬆️
... and 2 more

... and 607 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1734d1b...6fac4d6. Read the comment docs.

github-actions · 2024-06-20T20:28:18Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1208.50 ms	1221.08 ms	12.58 ms
Size	21.58 KiB	671.08 KiB	649.50 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`52e4912`	1216.25 ms	1226.69 ms	10.44 ms
`742d4b6`	1230.41 ms	1247.23 ms	16.83 ms
`297f460`	1234.81 ms	1255.27 ms	20.45 ms
`becc941`	1221.90 ms	1240.37 ms	18.47 ms
`340fb46`	1224.60 ms	1239.27 ms	14.67 ms
`3297d6e`	1195.69 ms	1212.35 ms	16.65 ms
`0d32275`	1215.31 ms	1240.19 ms	24.88 ms
`4d3df92`	1218.27 ms	1248.30 ms	30.03 ms
`b15521e`	1224.44 ms	1251.13 ms	26.68 ms
`e5dcbd5`	1223.47 ms	1243.90 ms	20.43 ms

App size

Revision	Plain	With Sentry	Diff
`52e4912`	21.58 KiB	418.14 KiB	396.56 KiB
`742d4b6`	21.58 KiB	546.20 KiB	524.62 KiB
`297f460`	21.58 KiB	629.83 KiB	608.24 KiB
`becc941`	21.58 KiB	419.82 KiB	398.24 KiB
`340fb46`	21.58 KiB	418.78 KiB	397.20 KiB
`3297d6e`	21.58 KiB	418.44 KiB	396.86 KiB
`0d32275`	22.84 KiB	403.14 KiB	380.29 KiB
`4d3df92`	22.85 KiB	413.45 KiB	390.60 KiB
`b15521e`	21.58 KiB	573.18 KiB	551.60 KiB
`e5dcbd5`	22.85 KiB	414.09 KiB	391.24 KiB

philipphofmann · 2024-07-25T09:12:47Z

@armcknight, I guess after resolving the conflicts, I can review this?

Check MACH_PORT_VALID on return from pthread_mach_thread_np

4b8cfd7

pthread_mach_thread_np can return invalid values so check for those and return nullptr from ThreadHandle

indragiek requested review from philipphofmann, brustolin and armcknight as code owners December 27, 2023 20:52

Update CHANGELOG.md

b656bde

indragiek force-pushed the indragiek/pthread_mach_thread_np-null branch from 6990048 to b656bde Compare December 27, 2023 21:16

Format code

c4496ee

philipphofmann reviewed Jan 2, 2024

View reviewed changes

Sources/Sentry/SentryThreadHandle.cpp Outdated Show resolved Hide resolved

Sources/Sentry/include/SentryThreadHandle.hpp Outdated Show resolved Hide resolved

Merge branch 'main' into indragiek/pthread_mach_thread_np-null

485dc2b

armcknight reviewed Jan 8, 2024

View reviewed changes

Sources/Sentry/SentryThreadHandle.cpp Outdated Show resolved Hide resolved

Sources/Sentry/include/SentryThreadHandle.hpp Outdated Show resolved Hide resolved

armcknight mentioned this pull request Jan 9, 2024

fix: check for null value returned from pthread_mach_thread_np #3443

Closed

armcknight mentioned this pull request Jun 14, 2024

fix: guard against null thread handle for profiler sampling thread #4076

Merged

philipphofmann reviewed Jun 17, 2024

View reviewed changes

fix: guard against null thread handle for profiler sampling thread (#…

6fac4d6

…4076)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check MACH_PORT_VALID on return from pthread_mach_thread_np #3520

Check MACH_PORT_VALID on return from pthread_mach_thread_np #3520

indragiek commented Dec 27, 2023

philipphofmann left a comment •

edited

Loading

armcknight left a comment

philipphofmann left a comment

codecov bot commented Jun 20, 2024 •

edited

Loading

github-actions bot commented Jun 20, 2024

Baseline results on branch: main

Startup times

App size

philipphofmann commented Jul 25, 2024

Check MACH_PORT_VALID on return from pthread_mach_thread_np #3520

Are you sure you want to change the base?

Check MACH_PORT_VALID on return from pthread_mach_thread_np #3520

Conversation

indragiek commented Dec 27, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

philipphofmann left a comment • edited Loading

Choose a reason for hiding this comment

armcknight left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 20, 2024

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

philipphofmann commented Jul 25, 2024

philipphofmann left a comment •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading